-
Notifications
You must be signed in to change notification settings - Fork 279
(wrong branch - feta) #5901
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(wrong branch - feta) #5901
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4ee59259c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| +import Foundation | ||
| +import StoreKit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import UIKit so UIApplication is in scope
The new Swift helper uses UIApplication/UIWindowScene in requestReview, but StoreReview.swift only imports Foundation and StoreKit here. Swift doesn’t re-export UIKit from StoreKit, so this will fail to compile with “Cannot find 'UIApplication' in scope” when the patch is applied. Add import UIKit to ensure the symbols are available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| const reduxCameraPermission = useSelector(state => state.permissions.camera) | ||
| const [cameraPermission, setCameraPermission] = React.useState( | ||
| reduxCameraPermission | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local permission state doesn't sync with Redux updates
Medium Severity
The cameraPermission local state is initialized from reduxCameraPermission but never syncs with Redux updates after mount. When a user grants camera permission via OS Settings and returns to the app, PermissionsManager updates Redux, but the local cameraPermission state remains stale. This leaves users stuck on the permission-denied view even after granting permission, requiring them to close and reopen the modal.
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is the wrong solution. We don't want to track state locally in the component - that's the job of the permission subsystem. The problem is that the permission subsystem is modifying things and then forgetting to update redux.
Over in the permissions manager, we should do this:
/**
* Checks permission and attempts to request permissions (only if checked
* permission was 'denied')
*/
export const checkAndRequestPermission =
(permission: Permission): ThunkAction<Promise<PermissionStatus>> =>
async dispatch => {
const status: PermissionStatus = await check(permissionNames[permission])
if (status !== 'denied') return status
const newStatus = await request(permissionNames[permission])
if (newStatus !== status) {
dispatch({
type: 'PERMISSIONS/UPDATE',
data: { [permission]: newStatus }
})
}
return newStatus
}And then here is just becomes:
- checkAndRequestPermission('camera')
+ dispatch(checkAndRequestPermission('camera'))And presto! The permissions manager is updating redux so this scene works correctly.
Package is not being maintained as of 2023. A forked branch and PR addresses iOS 18+ support: oblador/react-native-store-review@575f492 - The dependency now points to the above fork/SHA - Patched to add the missing Swifth module configuration to the podspec, and to import using angle brackets in the swift code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| import FastImage from 'react-native-fast-image' | ||
| import Animated from 'react-native-reanimated' | ||
| import { useSafeAreaFrame } from 'react-native-safe-area-context' | ||
| import { requestReview } from 'react-native-store-review/src' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong import bypasses review logic, breaks Android
High Severity
The requestReview function is imported directly from react-native-store-review/src instead of using the existing wrapper in src/actions/RequestReviewActions.tsx. The native module only works on iOS, leaving Android users without in-app review functionality. Additionally, this bypasses the app's sophisticated review trigger system that tracks user actions, respects cooldown periods, and provides Android fallback handling with a modal dialog.
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, what Cursor said. Is this a debugging commit for QA? In that case, just be sure to drop it before merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is for the wrong branch - this is the feta test build to allow QA to test easily.
src/components/modals/ScanModal.tsx
Outdated
| checkAndRequestPermission('camera').catch(err => { | ||
| showError(err) | ||
| }) | ||
| checkAndRequestPermission('camera').catch(showError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we can't just do .catch(showError), because then ScanModal.ts won't appear in the stack trace. We have to do .catch((error: unknown) => showError(error)), even though that's way more annoying.
| .catch(error => { | ||
| showDevError(error) | ||
| }) | ||
| .catch(showDevError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
| ).catch(err => { | ||
| showError(err) | ||
| }) | ||
| ).catch(showError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
| const handleClose = (): void => { | ||
| triggerHaptic('impactLight') | ||
| // @ts-expect-error | ||
| // @ts-expect-error - AirshipBridge expects string | undefined but resolve() with no args is valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, would bridge.resolve(undefined) allow us to get rid of the // @ts-expect-error altogether?
| (peepholeSpaceLayout.height - holeSize) / 2 | ||
|
|
||
| const renderModalContent = () => { | ||
| const renderModalContent = (): React.JSX.Element | null => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React.ReactElement is the preferred return type.
| const reduxCameraPermission = useSelector(state => state.permissions.camera) | ||
| const [cameraPermission, setCameraPermission] = React.useState( | ||
| reduxCameraPermission | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is the wrong solution. We don't want to track state locally in the component - that's the job of the permission subsystem. The problem is that the permission subsystem is modifying things and then forgetting to update redux.
Over in the permissions manager, we should do this:
/**
* Checks permission and attempts to request permissions (only if checked
* permission was 'denied')
*/
export const checkAndRequestPermission =
(permission: Permission): ThunkAction<Promise<PermissionStatus>> =>
async dispatch => {
const status: PermissionStatus = await check(permissionNames[permission])
if (status !== 'denied') return status
const newStatus = await request(permissionNames[permission])
if (newStatus !== status) {
dispatch({
type: 'PERMISSIONS/UPDATE',
data: { [permission]: newStatus }
})
}
return newStatus
}And then here is just becomes:
- checkAndRequestPermission('camera')
+ dispatch(checkAndRequestPermission('camera'))And presto! The permissions manager is updating redux so this scene works correctly.
| import FastImage from 'react-native-fast-image' | ||
| import Animated from 'react-native-reanimated' | ||
| import { useSafeAreaFrame } from 'react-native-safe-area-context' | ||
| import { requestReview } from 'react-native-store-review/src' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, what Cursor said. Is this a debugging commit for QA? In that case, just be sure to drop it before merge.
|
Closing this since it was opened on a test build and referencing in relevant PR: #5906 |
PR opened on wrong branch
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have:
Note
Fix iOS in‑app reviews (18+)
react-native-store-reviewto a GitHub fork and apply a patch: addDEFINES_MODULE/SWIFT_VERSIONin podspec and switch to framework header import (RNStoreReview-Swift.h) for iOS build compatibilityPodfile.lockandyarn.lockto the patched modulerequestReview()inHomeSceneto surface the in‑app review promptfixed: In-app review for iOS 18+ScanModalusesSceneHeaderUi4, improves camera permission handling and typing, and adjusts header spacingWritten by Cursor Bugbot for commit 7a4ce5b. This will update automatically on new commits. Configure here.